Skip to content

Do not deduce parameter attributes during CTFE#151842

Merged
rust-bors[bot] merged 2 commits intorust-lang:mainfrom
eggyal:skip-deducing-parameter-attrs-during-ctfe
Mar 5, 2026
Merged

Do not deduce parameter attributes during CTFE#151842
rust-bors[bot] merged 2 commits intorust-lang:mainfrom
eggyal:skip-deducing-parameter-attrs-during-ctfe

Conversation

@eggyal
Copy link
Contributor

@eggyal eggyal commented Jan 29, 2026

View all comments

Ever since #103172, fn_abi_of_instance might look at a function's body to deduce certain codegen optimization attributes for its indirectly-passed parameters beyond what can be determined purely from its signature (namely today ArgAttribute::ReadOnly and ArgAttribute::CapturesNone). Since #130201, looking at a function's body in this way entails generating, for any coroutine-closures, additional by-move MIR bodies (which aren't represented in the HIR)—but this requires knowing the types of their context and consequently cycles can ensue if such bodies are generated before typeck is complete (such as during CTFE).

Since they have no bearing on the evaluation result, this patch breaks a subquery out from fn_abi_of_instance, fn_abi_of_instance_no_deduced_attrs, which returns the ABI before such parameter attributes are deduced; and that new subquery is used in CTFE instead (however, since parameter attributes are only deduced in optimized builds, as a performance optimization we avoid calling the original query unless we are performing such a build).

Fixes #151748
Fixes #152497

@rustbot
Copy link
Collaborator

rustbot commented Jan 29, 2026

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 29, 2026
@eggyal eggyal force-pushed the skip-deducing-parameter-attrs-during-ctfe branch from 5650820 to ea095f5 Compare January 29, 2026 22:26
@rust-log-analyzer

This comment has been minimized.

@eggyal

This comment was marked as outdated.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 29, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@eggyal eggyal force-pushed the skip-deducing-parameter-attrs-during-ctfe branch from ea095f5 to de839fc Compare January 30, 2026 06:50
@RalfJung
Copy link
Member

Note that ABI "adjustments" typically do more than just deducing parameter attributes. So if "unadjusted" just refers to the attributes, that's confusing naming.

Also, having the full signature might be relevant for the UB checking that const-eval and Miri are doing. So it's at least slightly concerning that we shouldn't be able to access some of that info any more. What exactly is the difference between the signatures we obtain this way?

@eggyal

This comment was marked as outdated.

@RalfJung
Copy link
Member

RalfJung commented Jan 30, 2026

Looking at #151748, it seems like the underlying issue is that fn_sig_of_instance looks at the body of the function. That is indeed somewhat sus -- at least all the UB-relevant information should be deducible without the body. Please extend the PR description to explain this; it currently takes a bit of digging to fiure out what problem you're actually solving.

If we end up resolving this by splitting the query, it is very important that the queries have good documentation explaining their relationship. We want to avoid someone changing this code in the future in a way that removes too much information from the "unadjusted" signature.

@eggyal

This comment was marked as outdated.

@eggyal

This comment was marked as outdated.

@eggyal eggyal force-pushed the skip-deducing-parameter-attrs-during-ctfe branch from de839fc to 6a5ec89 Compare January 30, 2026 08:19
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 30, 2026
@rust-log-analyzer

This comment has been minimized.

@eggyal eggyal force-pushed the skip-deducing-parameter-attrs-during-ctfe branch from 6a5ec89 to 132a633 Compare January 30, 2026 08:27
@RalfJung
Copy link
Member

That name and PR description are much better, thank you!

@RalfJung
Copy link
Member

@bors try
@rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot pushed a commit that referenced this pull request Jan 30, 2026
…tfe, r=<try>

Do not deduce parameter attributes during CTFE
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 30, 2026
@RalfJung
Copy link
Member

I gave some feedback on the comments. I have not looked at the actual code changes at all.

@RalfJung
Copy link
Member

However it relies on downstream code never invoking the fn_abi_of_instance_query query directly

FWIW this is something we already do quite a bit; we usually call these queries something_raw.

@eggyal
Copy link
Contributor Author

eggyal commented Feb 22, 2026

@rustbot ready

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's much better, thank you. :)

Now that I can see through the code better, I did notice two more things.

View changes since this review

@rust-bors

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 1, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@eggyal
Copy link
Contributor Author

eggyal commented Mar 1, 2026

All good points—I've incorporated, and rebased.

@rustbot ready

@RalfJung
Copy link
Member

RalfJung commented Mar 4, 2026

One last comment nit. :) r=me with that resolved. Thanks a lot for working through so many rounds of review with me!
@bors delegate+

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 4, 2026

✌️ @eggyal, you can now approve this pull request!

If @RalfJung told you to "r=me" after making some further change, then please make that change and post @bors r=RalfJung.

eggyal added 2 commits March 4, 2026 16:38
`fn_abi_of_instance` can look at function bodies to deduce codegen
optimization attributes (namely `ArgAttribute::ReadOnly` and
`ArgAttribute::CapturesNone`) of indirectly-passed parameters.  This can
lead to cycles when performed during typeck, when such attributes are
irrelevant.

This patch breaks a subquery out from `fn_abi_of_instance`,
`fn_abi_of_instance_no_deduced_attrs`, which returns the ABI before such
parameter attributes are deduced; and that new subquery is used in CTFE
instead.
As a performance optimization, we altogether avoid ever invoking the
query that computes an Instance fn's fully optimized ABI in cases where
the build will not actually deduce parameter attributes.
@eggyal
Copy link
Contributor Author

eggyal commented Mar 4, 2026

I updated the comment to reflect your suggestion, but with a minor difference in wording. Hope that's okay?

@bors r=RalfJung

Thanks a lot for working through so many rounds of review with me!

Au contraire - thank you for putting up with my constant changes, and providing such excellent feedback. This PR is so much better as a result!

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 4, 2026

📌 Commit f460bb1 has been approved by RalfJung

It is now in the queue for this repository.

@JonathanBrouwer
Copy link
Contributor

@bors p=1000
This might spuriously fail with the same disk space errors as in #153416, we'll see

@rust-bors

This comment has been minimized.

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 4, 2026

💔 Test for 8f8f174 failed: CI. Failed job:

@eggyal
Copy link
Contributor Author

eggyal commented Mar 4, 2026

The runner has received a shutdown signal.

😆 sounds like GitHub's having a good day !

@JonathanBrouwer
Copy link
Contributor

Lmao lets try again then
@bors retry

@rust-bors

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu-llvm-20-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
/dev/sda15       98M  6.4M   92M   7% /boot/efi
tmpfs           1.6G   16K  1.6G   1% /run/user/1001
================================================================================

Sufficient disk space available (120307164KB >= 52428800KB). Skipping cleanup.
##[group]Run src/ci/scripts/setup-environment.sh
src/ci/scripts/setup-environment.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
---
set -ex

# Run a subset of tests. Used to run tests in parallel in multiple jobs.

# When this job partition is run as part of PR CI, skip tidy to allow revealing more failures. The
# dedicated `tidy` job failing won't block other PR CI jobs from completing, and so tidy failures
# shouldn't inhibit revealing other failures in PR CI jobs.
if [ "$PR_CI_JOB" == "1" ]; then
  echo "PR_CI_JOB set; skipping tidy"
  SKIP_TIDY="--skip tidy"
fi

../x.py --stage 2 test \
  ${SKIP_TIDY:+$SKIP_TIDY} \
  --skip compiler \
  --skip src
#!/bin/bash

set -ex

# Run a subset of tests. Used to run tests in parallel in multiple jobs.

# When this job partition is run as part of PR CI, skip tidy to allow revealing more failures. The
# dedicated `tidy` job failing won't block other PR CI jobs from completing, and so tidy failures
# shouldn't inhibit revealing other failures in PR CI jobs.
if [ "$PR_CI_JOB" == "1" ]; then
  echo "PR_CI_JOB set; skipping tidy"
  SKIP_TIDY="--skip tidy"
fi

../x.py --stage 2 test \
  ${SKIP_TIDY:+$SKIP_TIDY} \
  --skip tests \
  --skip coverage-map \
  --skip coverage-run \
  --skip library \
  --skip tidyselftest

@RalfJung
Copy link
Member

RalfJung commented Mar 4, 2026

@bors retry The runner has received a shutdown signal

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 4, 2026

❗ You can only retry pull requests that are approved and have a previously failed auto build.

Hint: There is currently a pending auto build on this PR. To cancel it, run @bors cancel.

@RalfJung
Copy link
Member

RalfJung commented Mar 4, 2026

Oh that was probably just a slow log analyzer?

@JonathanBrouwer
Copy link
Contributor

See #t-infra > free disk space step takes 8-10 minutes @ 💬 for discussion around disk space failures

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 5, 2026

☀️ Test successful - CI
Approved by: RalfJung
Duration: 3h 55m 22s
Pushing b55e20a to main...

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing b90dc1e (parent) -> b55e20a (this PR)

Test differences

Show 82 test diffs

Stage 1

  • [ui] tests/ui/coroutine/avoid-query-cycle-in-ctfe.rs: [missing] -> pass (J1)

Stage 2

  • [ui] tests/ui/coroutine/avoid-query-cycle-in-ctfe.rs: [missing] -> pass (J0)

Additionally, 80 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard b55e20ad905a336395ca80863488d0827712d067 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-android: 28m 28s -> 20m 37s (-27.6%)
  2. aarch64-apple: 3h 15m -> 3h 48m (+17.3%)
  3. dist-apple-various: 1h 48m -> 2h 5m (+15.9%)
  4. dist-x86_64-msvc: 1h 51m -> 2h 4m (+11.7%)
  5. dist-ohos-armv7: 1h 12m -> 1h 20m (+11.2%)
  6. dist-aarch64-llvm-mingw: 1h 31m -> 1h 41m (+10.6%)
  7. aarch64-gnu-debug: 1h 8m -> 1h 14m (+9.8%)
  8. x86_64-msvc-2: 2h 30m -> 2h 15m (-9.5%)
  9. dist-ohos-aarch64: 1h 16m -> 1h 23m (+9.4%)
  10. x86_64-msvc-ext3: 1h 42m -> 1h 51m (+8.8%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b55e20a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.1% [-0.1%, -0.1%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -0.2%, secondary -1.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.0% [2.0%, 2.0%] 1
Regressions ❌
(secondary)
5.7% [5.7%, 5.7%] 1
Improvements ✅
(primary)
-2.4% [-2.4%, -2.4%] 1
Improvements ✅
(secondary)
-5.5% [-7.5%, -3.6%] 2
All ❌✅ (primary) -0.2% [-2.4%, 2.0%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 479.576s -> 478.218s (-0.28%)
Artifact size: 397.00 MiB -> 394.94 MiB (-0.52%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

regression/1.93: async closures - error[E0391]: cycle detected when evaluating type-level constant 1.93 regression satisfying send obligation